Skip to content

test(uds): drive functional response windows from an injected clock - #183

Merged
dborgards merged 11 commits into
mainfrom
feature/test-itime-source-uds-functional
Sep 27, 2026
Merged

dborgards merged 11 commits into
mainfrom
feature/test-itime-source-uds-functional

Conversation

@dborgards

@dborgards dborgards commented Sep 27, 2026 •

Copy link
Copy Markdown
Owner

What does this change?

Functional UDS windows and J1939-TP deadlines were armed on the wall clock, so a slow runner could push a frame across a window the test thought it was still inside. That is the macOS failure of A_Late_Negative_Answer_To_A_Suppressed_Send_Does_Not_Land_In_The_Next_Window (run 36234735257, ContainSingle).

J1939TpChannel now takes an optional ProtocolActor, the same way the J1939 node does. It owns that actor only when it created one. J1939Tp.Open is unchanged, and a node still does not hand its own actor to a TP channel it opens internally.

Functional ISO-TP collection and UdsFunctionalClient take the same optional actor. Null keeps CancelAfter and Stopwatch. A test that passes an actor also stamps host arrival and handoff from that actor's clock, through an internal CanBusService overload, so a deadline and a frame stamp stay comparable.

Refs #171. First slice only. Whether the rest of #171 is a hardening track or release-blocking is the issue's open decision question; this pull request does not close it.

Injection shape

  • internal J1939TpChannel(..., ProtocolActor? actor = null). No separate ITimeSource parameter: timers read the actor's clock, as J1939NodeImpl does. Production default is still new ProtocolActor().
  • J1939TpChannel.Dispose never disposes a borrowed actor. The maintainer chose to harden this path here rather than align it with IsoTpChannel / J1939NodeImpl.
    • It first joins the reader and closes the subscription.
    • From then on, HandleIncoming drops any frame the actor reaches, whenever it gets to it. A reader that outlives its join therefore cannot open a session behind the cleanup.
    • With an owned actor, disposing the actor runs the cleanup.
    • With a borrowed actor, it posts the cleanup and waits up to 2 s. If the cleanup runs in time, every in-flight and queued send has failed when Dispose returns.
    • If the borrowed actor is still busy after 2 s, Dispose reports a TimeoutException on BackgroundExceptionOccurred and returns. An owned service is disposed only after the cleanup has run.
    • On the actor's own loop, where waiting would deadlock, it fails the sessions inline.
    • If the borrowed actor is already disposed, it leaves that actor's sessions alone.
  • internal IsoTpFunctionalClient(..., ProtocolActor? clock = null) and the listener it opens. IsoTp.OpenFunctional is unchanged.
  • internal UdsFunctionalClient.Create(client, ProtocolActor? clock, ...). The public Create delegates to it with a null clock, so the P2 / P2* defaults are in one place.
  • internal CanBusService(ICanBus bus, Func<long>? hostTimestamp). RawCan does not reference the actor assembly. The public constructor is unchanged.
  • FunctionalWindow ends a collection window. On an injected actor, its timer callback (End) and its Dispose take one lock. A callback the loop picked up before the timer was cancelled finds the window disposed and does nothing. Disposal does not go through the actor, so an actor that is already torn down makes no difference.
  • UdsFunctionalClient.DelayListenerStart(TimeSpan) is the test hook that was the ListenerStartDelay property. It throws on a client that has no injected clock, because on the wall clock it would be a sleep.

Tests moved onto the clock

In UdsFunctionalClientTests: the late-negative flake and the other functional-window cases that place a frame inside or past P2 / P2* (A_Late_Negative_Answer_To_A_Previous_Request_Does_Not_Land_In_The_Next_Window, A_Cancelled_Collection_Still_Leaves_Its_Window_For_The_Next_Call, A_Pending_Answer_Collected_After_P2_Does_Not_Revive_The_Window, A_Listener_Starting_After_The_Window_Still_Hears_What_It_Buffered, A_Cancelled_Collection_Does_Not_Lose_The_Pending_Answer_The_Listener_Heard, A_Cancelled_Wait_Leaves_The_Listener_To_Hear_The_Pending_Answer, A_Pending_Answer_In_The_Gap_After_A_Suppressed_Send_Is_Observed, A_Collection_That_Outlasts_The_Window_Does_Not_Leave_A_Zombie_Listener).

Bam_Packet_Spacing_Follows_The_Injected_Actors_Clock checks that wall time does not release the next TP.DT while the injected clock is frozen, and that advancing it does. A failed construction disposes an actor the channel created and leaves an injected one running.

New tests for the seam's own branches

Each was checked by breaking its line in the source and watching it fail.

  • Functional_Collect_On_An_Injected_Clock_Drains_By_That_Clocks_Deadline: the drain after the window ends keeps a frame stamped at the deadline and drops one a tick later, plus an unstamped one.
  • An_Unstamped_Functional_Response_Is_Stamped_From_The_Collectors_Clock.
  • A_Clocked_Window_Ends_On_Its_Clock_And_Outlives_Its_Actor: the window ends when its clock says so; disposing it after its actor, then running a late callback, throws nothing.
  • A_Suppressed_Send_Cancelled_After_The_Driver_Took_It_Leaves_P2_From_The_Cancellation.
  • A_Suppressed_Send_Without_A_Transmit_Stamp_Is_Anchored_At_Its_Confirmation.
  • A_Listener_Start_Delay_Needs_An_Injected_Clock.
  • The borrowed-actor dispose paths, each with an in-flight BAM and a BAM queued behind it: Disposing_On_A_Busy_Borrowed_Actor_Fails_The_Send_Before_It_Returns, Disposing_On_A_Borrowed_Actor_Stuck_Past_The_Budget_Defers_The_Service, Disposing_From_The_Borrowed_Actors_Loop_Fails_The_Send_At_Once, Disposing_After_The_Borrowed_Actor_Leaves_Its_Sessions_To_It.
  • Frames_The_Actor_Reaches_After_Dispose_Began_Are_Dropped: a complete BAM that the reader has already handed to a busy borrowed actor is not delivered once Dispose has begun.

What stays for follow-up

  • Every other category-2 row in docs/reviews/2026-09-25-delay-sleep-audit.md: J1939 node backoff, the remaining J1939-TP timer sleeps (the seam is in place; those tests are not converted), ISO-TP, CANopen, raw CAN, and the physical UDS client's P2 rows in UdsClientTests.
  • In UdsFunctionalClientTests, the sleeps that are not that window: the queue delay in A_Call_Queued_Behind_Another_Does_Not_Send_After_Dispose, the bus-pump delay in An_Invalid_Collection_Window_Transmits_Nothing, and the driver holds in the acceptance-anchor tests (Task.Delay(200), Task.Delay(2000), Task.Delay(1100), and the 150 ms negative that rides that hold). Those holds are a wall-clock gap across a stuck driver. Freezing the protocol clock would erase what they measure.
  • The 50 ms lock-setup delay in A_Pending_Answer_From_Before_The_Handoff_Is_Not_This_Requests.

Type of change

  • docs / test / refactor / chore / ci — no release

Every commit is refactor(...) or test(...), apart from the CodeQL autofix commit. That one has no Conventional Commit type, so the release analyser ignores it. None of the commits is a feat or a fix, so this pull request does not publish a release.

Checklist

  • dotnet build CanKit.Pro.sln -c Release -p:CI=true succeeds
  • dotnet test CanKit.Pro.sln -c Release --framework net10.0 passes
  • dotnet format --verify-no-changes and dotnet pack + eng/verify-packages.py pass
  • Public API unchanged; the new parameters are internal and commented
  • New behaviour is covered by a test
  • Refs Replace wall-clock category-2 test sleeps (macOS flake risk) #171

🤖 Generated with Claude Code

https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s

cursoragent and others added 2 commits September 27, 2026 11:42
…r's clock

The channel always built its own ProtocolActor, so BAM spacing and the other
TP deadlines could only be waited out on the wall clock. An optional actor
follows the node: the channel owns the loop only when it created it, and
production still constructs one when the caller passes none.

Co-authored-by: Dietmar Borgards <dborgards@users.noreply.github.com>
Functional P2, P2* and the ISO-TP collection window were Stopwatch plus
CancelAfter, which is how a late negative on a busy macOS runner landed in
the next window. Tests that build the stack can pass one actor, and the
demux stamps frames from that same clock. A null clock keeps the wall-clock path.

Co-authored-by: Dietmar Borgards <dborgards@users.noreply.github.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-27T15:18:56.063468Z 5f84e17 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Comment thread src/CanKit.Pro.IsoTp/IsoTpFunctionalListener.cs Fixed
@codecov

codecov Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

dborgards and others added 2 commits September 27, 2026 14:45
… empty catch block'

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Codecov reported 12 uncovered or partial lines in the functional window
seam of this branch. Each is now either exercised by a test that fails
when the line is broken, or gone:

- FunctionalWindow cancels a source of its own that is never disposed,
  so a timer callback racing the window's disposal cannot meet a
  disposed source. That removes the ObjectDisposedException catch the
  CodeQL autofix left behind, rather than papering over the race.
- TryParseFunctionalResponse takes the collector's clock as required:
  every caller passes one, so the Stopwatch fallback was dead.
- The public UdsFunctionalClient.Create delegates to the internal
  overload with a null clock, so the defaults live in one place.
- The listener start delay is a method that refuses a wall clock; the
  wall-clock Task.Delay branch it replaced had no remaining caller.
- New tests: the post-window drain on an injected clock, the unstamped
  arrival fallback, a suppressed send cancelled after the driver took
  it, a suppressed send whose driver reports no transmit stamp, and the
  start-delay guard. Each was checked by mutating its line.

Refs #171

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s

Copy link
Copy Markdown
Owner Author

@codex review

33382e7 closes the Codecov patch gaps on this branch. Codex last reviewed 6554993, so this asks for a review of the current head.


Generated by Claude Code

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 33382e72c1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/CanKit.Pro.IsoTp/IsoTpFunctionalListener.cs Fixed
CodeQL flagged the source the clocked FunctionalWindow never disposed.
Disposing it directly would race a timer callback the loop has already
taken, since cancelling the timer only flags it. The window now has one
linked source again: the timer cancels it on the actor, and Dispose posts
the disposal to that same actor, so the two cannot overlap and nothing
is left undisposed.

Refs #171

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s

Copy link
Copy Markdown
Owner Author

@codex review

482f7d1 changes how FunctionalWindow disposes its source on an injected actor, in response to CodeQL alert 404.


Generated by Claude Code

@cursor

cursor Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches dispose ordering and session cleanup on J1939-TP and shared timing across demux stamps and protocol deadlines; behavior is heavily tested but lifecycle edge cases on borrowed actors are subtle.

Overview
Adds an optional ProtocolActor test seam so ISO-TP functional collection windows, UDS functional P2/P2*, and J1939-TP timers can be advanced by hand instead of wall-clock sleeps (#171). Production behavior is unchanged when the actor is null (CancelAfter / Stopwatch).

ISO-TP / UDS: IsoTpFunctionalClient and IsoTpFunctionalListener take an optional clock; collection uses new internal FunctionalWindow (actor Schedule vs CancelAfter). Deadlines and unstamped arrivals use ITimeSource. UdsFunctionalClient.Create gains an internal overload with the same clock; DelayListenerStart replaces the wall-clock ListenerStartDelay property and throws without an injected clock. CanBusService adds an internal constructor so host arrival/handoff stamps use the same clock as the window.

J1939-TP: J1939TpChannel can borrow an injected actor (like the J1939 node). Dispose joins the reader and closes the subscription before session cleanup, never disposes a borrowed actor, waits up to 2s for cleanup on a busy borrowed actor (or times out and defers owned-service disposal), and HandleIncoming drops frames once dispose has started (#183).

Tests: Flaky functional-window cases move onto VirtualClock; new coverage for injected-clock drain, FunctionalWindow vs disposed actor, J1939 BAM spacing, and borrowed-actor dispose paths. InternalsVisibleTo extended to IsoTp and Uds.

Reviewed by Cursor Bugbot for commit 5f84e17. Bugbot is set up for automated code reviews on this repo. Configure here.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 482f7d1f1d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/CanKit.Pro.IsoTp/IsoTpFunctionalListener.cs Outdated

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread src/CanKit.Pro.IsoTp/IsoTpFunctionalListener.cs Outdated
Posting the linked source's disposal to the injected actor threw
ObjectDisposedException once that actor was gone. Client disposal does
not wait for listener tasks, so a collection can unwind after the actor
has been disposed (Codex and Bugbot on #183). The window now takes a
lock around its timer callback and its disposal. A callback the loop
picked up before the timer was cancelled finds the window disposed and
does nothing, and disposal no longer needs the actor to be running.

Refs #171

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s

Copy link
Copy Markdown
Owner Author

@codex review

cfe03a4 replaces the actor-posted disposal from 482f7d1 with a lock around the window's timer callback and its Dispose.


Generated by Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cfe03a48b3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/CanKit.Pro.J1939Tp/J1939TpChannel.cs Outdated
…pose

An owned actor runs the posted session cleanup as it is disposed, but an
injected one keeps running. If it was busy with the caller's other work,
Dispose returned with sends still in flight and the service was torn
down underneath them (Codex on #183). Dispose now waits up to 2 s for
the cleanup on a borrowed actor. It fails the sessions inline when it
runs on the actor's own loop, where waiting would deadlock, and when the
actor is already disposed, where no loop is left to do it.

Refs #171

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
Comment thread tests/CanKit.Pro.Tests/TestCases/J1939TpTests.cs

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 16c2c5c314

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/CanKit.Pro.J1939Tp/J1939TpChannel.cs Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 16c2c5c. Configure here.

Comment thread src/CanKit.Pro.J1939Tp/J1939TpChannel.cs Outdated
Comment thread src/CanKit.Pro.J1939Tp/J1939TpChannel.cs
CodeQL alert 405: the late FunctionalWindow in the clocked-window test
was not disposed if an assertion threw first. The J1939-TP dispose tests
had the same shape, with a helper that built and returned the sender.
Each test now owns what it creates with `using`, and both Dispose
methods are idempotent.

Refs #171

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s

Copy link
Copy Markdown
Owner Author

@codex review

16c2c5c: J1939TpChannel.Dispose waits for the session cleanup on a borrowed actor. 3a53266 makes the new tests dispose what they create.


Generated by Claude Code

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 3a53266d1e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Three findings on 16c2c5c (Codex and Bugbot on #183):

- The reader is now joined and the subscription closed before the
  session cleanup is posted. A frame the reader was still handing to the
  actor is queued ahead of the cleanup, not behind it.
- When the injected actor is already disposed, the channel no longer
  fails its sessions from the calling thread. They are actor state, and
  its loop may still be draining.
- When the borrowed actor is still busy after 2 s, Dispose reports a
  TimeoutException on BackgroundExceptionOccurred. It disposes an owned
  service only once the cleanup has run, not underneath it.

Refs #171

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s
The channel disposes the service it owns, and the test's `using` covers
an assertion that throws before that. CanBusService.Dispose is
idempotent, so disposing twice is harmless. This is the same shape
CodeQL alert 406 flagged in the earlier helper.

Refs #171

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s

Copy link
Copy Markdown
Owner Author

@codex review

e949614 reorders J1939TpChannel.Dispose on a borrowed actor. It joins the reader before posting the cleanup, leaves a disposed actor's sessions alone, and handles the 2 s timeout explicitly by reporting it and deferring disposal of the owned service. ddae84d is test-only.


Generated by Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ddae84dc25

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/CanKit.Pro.J1939Tp/J1939TpChannel.cs Outdated
Comment thread src/CanKit.Pro.J1939Tp/J1939TpChannel.cs Fixed
A reader that outlived its 2 s join could still post HandleIncoming to a
borrowed actor after the session cleanup had run, and open a session
nobody fails (Codex on #183). HandleIncoming now returns immediately once
Dispose has begun, whenever the actor gets to the frame. The reader-join
catch is narrowed to AggregateException, which is what Task.Wait throws
for a faulted or cancelled reader (CodeQL alert 407).

Refs #171

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013WJ8h1ahw4Nj5dYuEWy34s

Copy link
Copy Markdown
Owner Author

@codex review

5f84e17: HandleIncoming drops any frame the actor reaches after Dispose has begun, and the reader-join catch is narrowed to AggregateException.


Generated by Claude Code

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: 5f84e17f54

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants